-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ser/deser for macro function #1846
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1846 +/- ##
==========================================
- Coverage 91.29% 91.23% -0.07%
==========================================
Files 792 796 +4
Lines 28762 28951 +189
==========================================
+ Hits 26259 26412 +153
- Misses 2503 2539 +36
☔ View full report in Codecov by Sentry. |
offset = SerDeser::deserializeValue<std::string>(value.name, fileInfo, offset); | ||
offset = SerDeser::deserializeValue<LogicalType>(value.dataType, fileInfo, offset); | ||
offset = SerDeser::deserializeValue<property_id_t>(value.propertyID, fileInfo, offset); | ||
void SerDeser::deserializeValue<Property>(Property& value, FileInfo* fileInfo, uint64_t& offset) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it necessary to change deserializeValue
to taking a reference of offset
here? My guess is for simplicity?
I think both serializeValue
and deserializeValue
should be consistent. If you are changing deserializeValue
, make sure you change serializeValue
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the serailizeValue function will return a unique_ptr when deserializing the parsedExpression.
1be05cf
to
d4b6c30
Compare
d4b6c30
to
eab2e25
Compare
Added serialization/deserialization for macro functions. Instead of using the template
serializeValue<>
anddeserializeValue<>
functions to serialize classes, classes should implement their own serialization/deserialization methods. Another refactor on the ser/deser APIs needs to be done.TODOs